Add Human Review Client Endpoints #211
Conversation
WalkthroughWalkthroughThe changes introduce new interfaces and methods in the Changes
Possibly related issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/client.ts (3 hunks)
- src/index.ts (1 hunks)
- src/testing/index.ts (1 hunks)
- src/testing/models.ts (1 hunks)
- src/types.ts (1 hunks)
Additional comments not posted (7)
src/index.ts (2)
4-11: LGTM!The export statement is syntactically correct and aligns with the changes mentioned in the AI-generated summary. The additional type exports enhance the type definitions available for import from
index.ts.
12-12: LGTM!The export statement is syntactically correct and aligns with the changes mentioned in the AI-generated summary. The additional type export enhances the type definitions available for import from
index.ts.src/types.ts (1)
45-51: LGTM!The new
HumanReviewFieldContentTypeenum is a valuable addition that enables type-safe handling of different content types in the context of human review processes. The enum provides a structured way to represent and work with various content formats.The enum follows good naming conventions, with uppercase constants. It is properly exported, making it available for use in other parts of the codebase.
Overall, the addition of this enum enhances the type definitions and does not introduce any breaking changes or negative impacts on existing code.
src/testing/index.ts (1)
16-16: LGTM!The change updates the export source for
HumanReviewFieldContentTypefrom./modelsto../types. This aligns with the alterations mentioned in the summary and should not have any negative impact on the functionality or usage of the exported entity.src/testing/models.ts (1)
2-2: LGTM!Moving the
HumanReviewFieldContentTypeenum to a central location in../typesis a good practice to promote reusability and avoid duplication across the codebase.src/client.ts (2)
72-76: Interfaces for Human Review Jobs are well-definedThe
HumanReviewJobinterface is appropriately structured.
78-81: Extension of HumanReviewJob with Test CasesThe
HumanReviewJobWithTestCasesinterface correctly extendsHumanReviewJoband adds thetestCasesproperty.
src/client.ts
Outdated
| export interface HumanReviewJobTestCaseResult { | ||
| id: string; | ||
| reviewerEmail: string; | ||
| status: 'Submitted' | 'Pending'; | ||
| grades: { name: string; grade: number }[]; | ||
| automatedEvaluations: { | ||
| id: string; | ||
| originalScore: number; | ||
| overrideScore: number; | ||
| overrideReason?: string; | ||
| }[]; | ||
| inputFields: { | ||
| id: string; | ||
| name: string; | ||
| value: string; | ||
| contentType: HumanReviewFieldContentType; | ||
| }[]; | ||
| outputFields: { | ||
| id: string; | ||
| name: string; | ||
| value: string; | ||
| contentType: HumanReviewFieldContentType; | ||
| }[]; | ||
| fieldComments: { | ||
| fieldId: string; | ||
| startIdx?: number; | ||
| endIdx?: number; | ||
| value: string; | ||
| inRelationToGradeName?: string; | ||
| inRelationToAutomatedEvaluationId?: string; | ||
| }[]; | ||
| inputComments: { | ||
| value: string; | ||
| inRelationToGradeName?: string; | ||
| inRelationToAutomatedEvaluationId?: string; | ||
| }[]; | ||
| outputComments: { | ||
| value: string; | ||
| inRelationToGradeName?: string; | ||
| inRelationToAutomatedEvaluationId?: string; | ||
| }[]; | ||
| } |
There was a problem hiding this comment.
Inconsistent representation of reviewer information
In HumanReviewJob, the reviewer is represented as an object with id and email:
reviewer: { id: string; email: string };However, in HumanReviewJobTestCaseResult, the reviewer is represented by a single reviewerEmail string property:
reviewerEmail: string;For consistency and future extensibility, consider representing the reviewer in HumanReviewJobTestCaseResult as an object similar to HumanReviewJob.
| public async getHumanReviewJobs(): Promise<{ jobs: HumanReviewJob[] }> { | ||
| return this.get('/human-review/jobs'); | ||
| } |
There was a problem hiding this comment.
Consider consistent return types for methods
The getHumanReviewJobs method returns Promise<{ jobs: HumanReviewJob[] }>, wrapping the array in an object. However, other methods like getViews() return arrays directly as Promise<View[]>.
For consistency, consider returning Promise<HumanReviewJob[]> directly, unless the API response necessitates the wrapping object.
src/client.ts
Outdated
| public async getHumanReviewJobs(): Promise<{ jobs: HumanReviewJob[] }> { | ||
| return this.get('/human-review/jobs'); | ||
| } | ||
|
|
||
| public async getHumanReviewJobTestCases( | ||
| jobId: string, | ||
| ): Promise<HumanReviewJobWithTestCases> { | ||
| return this.get(`/human-review/jobs/${jobId}/test-cases`); | ||
| } | ||
|
|
||
| public async getHumanReviewJobTestCaseResult( | ||
| jobId: string, | ||
| testCaseId: string, | ||
| ): Promise<HumanReviewJobTestCaseResult> { | ||
| return this.get(`/human-review/jobs/${jobId}/test-cases/${testCaseId}`); | ||
| } |
There was a problem hiding this comment.
Add unit tests for new API methods
Consider adding unit tests for the newly added methods to ensure they function correctly and handle potential errors.
src/client.ts
Outdated
| public async getHumanReviewJobTestCases( | ||
| jobId: string, | ||
| ): Promise<HumanReviewJobWithTestCases> { | ||
| return this.get(`/human-review/jobs/${jobId}/test-cases`); | ||
| } |
There was a problem hiding this comment.
Ensure proper encoding of jobId in URL
When constructing the URL in getHumanReviewJobTestCases, consider encoding jobId to handle any special characters that may be present.
Apply this diff to encode the jobId:
- return this.get(`/human-review/jobs/${jobId}/test-cases`);
+ return this.get(`/human-review/jobs/${encodeURIComponent(jobId)}/test-cases`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async getHumanReviewJobTestCases( | |
| jobId: string, | |
| ): Promise<HumanReviewJobWithTestCases> { | |
| return this.get(`/human-review/jobs/${jobId}/test-cases`); | |
| } | |
| public async getHumanReviewJobTestCases( | |
| jobId: string, | |
| ): Promise<HumanReviewJobWithTestCases> { | |
| return this.get(`/human-review/jobs/${encodeURIComponent(jobId)}/test-cases`); | |
| } |
src/client.ts
Outdated
| public async getHumanReviewJobTestCaseResult( | ||
| jobId: string, | ||
| testCaseId: string, | ||
| ): Promise<HumanReviewJobTestCaseResult> { | ||
| return this.get(`/human-review/jobs/${jobId}/test-cases/${testCaseId}`); | ||
| } |
There was a problem hiding this comment.
Ensure proper encoding of jobId and testCaseId in URL
In getHumanReviewJobTestCaseResult, to prevent issues with special characters in jobId and testCaseId, consider encoding these parameters when constructing the URL.
Apply this diff to encode the parameters:
- return this.get(`/human-review/jobs/${jobId}/test-cases/${testCaseId}`);
+ return this.get(`/human-review/jobs/${encodeURIComponent(jobId)}/test-cases/${encodeURIComponent(testCaseId)}`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async getHumanReviewJobTestCaseResult( | |
| jobId: string, | |
| testCaseId: string, | |
| ): Promise<HumanReviewJobTestCaseResult> { | |
| return this.get(`/human-review/jobs/${jobId}/test-cases/${testCaseId}`); | |
| } | |
| public async getHumanReviewJobTestCaseResult( | |
| jobId: string, | |
| testCaseId: string, | |
| ): Promise<HumanReviewJobTestCaseResult> { | |
| return this.get(`/human-review/jobs/${encodeURIComponent(jobId)}/test-cases/${encodeURIComponent(testCaseId)}`); | |
| } |
Summary by CodeRabbit
New Features
HumanReviewJob,HumanReviewJobWithTestCases, andHumanReviewJobTestCaseResult.Enhancements
HumanReviewFieldContentType, defining content types likeTEXT,MARKDOWN,HTML, andLINK.Refactor
HumanReviewFieldContentTypeto streamline type management across modules.